Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FIX: Member form validator fix. #11395

Merged
merged 2 commits into from
Sep 27, 2024
Merged

FIX: Member form validator fix. #11395

merged 2 commits into from
Sep 27, 2024

Conversation

mfendeksilverstripe
Copy link
Contributor

@mfendeksilverstripe mfendeksilverstripe commented Sep 20, 2024 •
edited
Loading

FIX: Member form validator fix.

Description

Fix for #11357

Manual testing steps

See #11357 for more details

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@GuySartorelli GuySartorelli mentioned this pull request Sep 25, 2024
Closed
2 tasks
GuySartorelli
GuySartorelli reviewed Sep 25, 2024
View reviewed changes
Copy link
Member

@GuySartorelli GuySartorelli left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good, thanks for submitting that.
Would you like to try adding a behat test for this, so we don't get the same regression again in the future?

That would need to target the silverstripe/silverstripe-admin repo.

@GuySartorelli GuySartorelli self-assigned this Sep 25, 2024
@mfendeksilverstripe
Copy link
Contributor Author

@GuySartorelli Added a unit test instead as a behat test seems like an overkill for this change.

GuySartorelli
GuySartorelli approved these changes Sep 27, 2024
View reviewed changes
Copy link
Member

@GuySartorelli GuySartorelli left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't tell us that the validation works as expected - but it's a good step in the right direction and probably enough for this specific PR.
Cheers.

We're having some trouble with CI at the moment, but I've tested this locally and it works as expected so I'm happy to merge.

It'll be included in the October minor release (okay to slip in there since it's a bug fix - and probably better there than as a patch for 5.2 because the behavioral change is fairly substantial)

@GuySartorelli GuySartorelli merged commit 8a538f7 into silverstripe:5.3 Sep 27, 2024
4 of 14 checks passed
@mfendeksilverstripe mfendeksilverstripe deleted the bugfix/member-validator branch September 27, 2024 02:55
@GuySartorelli GuySartorelli mentioned this pull request Sep 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuySartorelli GuySartorelli GuySartorelli approved these changes

Assignees

@GuySartorelli GuySartorelli

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mfendeksilverstripe @GuySartorelli